Users don't need to learn yet another term (chain id) or know what chain id Kovan has :)
I'd be more generic and talk about networks:
Network mismatch, please import a Parity Signer account for the XYZ Network.

ps: note that foundation translates to Ethereum mainnet outside of Parity.

Agreed. Although when the wording becomes longer the error message displays across 3 rows instead of 2 rows, such that the user has to scroll to read it. I assume we'll address this in a the separate PR that's dedicated to notifications #331 ?

yes, notification will be handled in their own PR for sure.

I've pushed a commit 1c24567 that addresses this

why? we don't need to be synced/good to query the chainId RPC

i just noticed that this approach was used elsewhere in the code when chainName was being used, so i just borrowed that approach thinking it was necessary

I think it's only used in Health.js, and the reason was we didn't want to make an RPC call before having set the API (since Health.js is always rendered). However, the AccountImportOptions screen can only be accessed through navigation inside the app, so the api will necessarily be set by then.

I've pushed commit 20e3088 that addresses this comment

nitpick but the chainId isn't an information inside the address like the function name could suggest, it's more like a meta data that comes alongside with the address

maybe currentChainId !== signerChainId and remove the function? we don't have to use bignumber here afaik

edit: ah, forgot that the api returns a BN. then we can use BigNumber(signerChainId).eq(currentChainId);

i've addressed this issue in commit c4cf7d5 and de3396f

as I wrote before I would remove this function altogether

i've addressed this issue in commit c4cf7d5

not related to this PR: it took me a while to understand what the function does, name is a bit confusing. maybe rename to accountAlreadyExists ?

i've addressed this in commit cab245e

cc @amaurymartiny the chainId$() rpc returns a bignumber, but AFAIK we could just return a regular int and it might simplify post treatment; no need for bignumber.

what's the consensus on this? are all numbers returned from @parity/api BigNumber? if so we should keep it this way; if not do we want to return regular ints for "simple" numbers? or would it be more confusing?

I think it could be a regular in too, but all the others RPC calls need to be BigNumber so it would be the only exception (i.e. to check balance, block number, transaction count, peer count, etc), so I agree with you that we should keep it as a BigNumber.

FYI, I had a look around and found that the chainId$ comes @paritytech/ light.js in https://github.com/paritytech/js-libs/blob/master/packages/light.js/src/rpc/eth.ts#L21, where the Typescript indicates returns type BigNumber | Symbol (in that file it looks like other calls also return this type), which calls from rpc/eth in @parity/ api https://github.com/paritytech/js-libs/blob/master/packages/api/src/rpc/eth/eth.js#L42 in light.js.
The API docs show that this type is returned here https://paritytech.github.io/js-libs/light.js/api/modules/_rpc_eth_.html
In the Parity Ethereum Rust codebase under rpc/eth the eth_chainId returns U64 https://github.com/paritytech/parity-ethereum/blob/master/rpc/src/v1/traits/eth.rs#L55

Sorry, I just saw this.

are all numbers returned from @parity/api BigNumber?

Yes, and I would keep it like that for consistency. Trick: instead of using .valueOf() .toNumber() or anything else, +chainId also works.

we can't deduce to what network (eth/etc) the address corresponds based solely on the address. so logic for address validation for eth/etc is the same. a valid eth address is a valid etc address

might be worth creating util functions somewhere
isEthChainId(chainId: number | BigNumber) : boolean (that accepts a number or a BN)
chainIdToString(chainId: number | BigNumber) : string

great idea! i've addressed it in this commit b31e433

nitpick: since we're modifying the lines, let's correct the grammar while we're at it and write 12 to 24-word seed phrase 😅

nitpick: ditto (not related to this PR) 11 or 12-word seed phrase (Parity Signer used to generate a 11-word recovery phrase)

or even ... an 11-word ...

it keeps on getting better

i've pushed commit 53a7c7e

not related to this PR: found a theoretical bug afaik

currently clear is async but called simply as this.clear(); in the other functions of the class, before they changed the properties to their new values. I would have assumed that this.clear() would be pushed on the event loop async stack and be run after the end of the calling function (and provoke a bug), however this is not the case; need to research why..

i.e. completely confused by the output of this:

let a = 3;

let clear = async () => {
 console.log('inside clear');
 a = 0;
}

let run = async () => {
 console.log('inside run');
 clear();
 console.log('inside run - continuing');
 a = 4;
}

run();
console.log('post run');

// output:
// inside run
// inside clear
// inside run - continuing
// post run

// would have expected, because of the way sync/async calls and promises are handled in js:
// post run
// inside run
// inside run - continuing
// inside clear

need to brush up on my js

all the same let's remove async from clear and make it a regular function!

ok, this explains that: The callback passed to a Promise constructor is always called synchronously, but the callbacks passed into then are always called asynchronously I had assumed promises callback always worked like setTimeout

I don't think we should reset isImport here, afaik clear() only resets the account info

i've pushed commit 1c8f7f2

that's weird

maybe try again if this.clear() doesn't reset isimport ?

it was because I was supplying it as await importFromSigner({ address, signerChainIdInt }); and then receiving it as importFromSigner = async ({ address, signerChainId }) => {.
So I've just renamed it to signerChainId to avoid confusion

BN.eq

Why do we check this?

I've changed it in this commit to make it clearer: b5296af

Sorry it was confusing code.... it was actually just so if the user is connected to the classic chain id and they enter an invalid Ethereum address, then it would show a tooltip that reminds them that they need to enter a valid Ethereum address, but it takes the opportunity to highlight to them that the recipient address should be an Ethereum Classic address so they don't lose their funds by sending it to a wrong address.

Perhaps a future PR could include an upfront warning on that page to remind the user to double check that the recipient address that they enter is on the chain as the chain that they're currently using, and potentially even check that for them automatically.

isNotErc20TokenAddress

isNotErc20TokenAddress

i'ved pushed in commit b94d4a2

currentChainIdBN.eq(signerChainId) is easier to read, and you don't even need to parseInt

i've addressed this in commit 0197108